Optionally allow redefinition of variable with different type #6197

JukkaL · 2019-01-15T17:05:25Z

If the flag --allow-redefinition is used, it's now possible to redefine a
variable with a different type. For example, this code will be okay:

x = 0  # Define 'x' with type 'int'
print(x)
x = 'a'  # Define another 'x' with type 'str'
print(x)

The variable needs to be read before redefinition is possible. This is mainly
to allow these idioms:

x: List[int]
x = []  # Not a redefinition

y = None  # type: List[int]  # Strict optional disabled
y = []  # Not a redefinition

It's also okay to use a type annotation in any of the assignments. This is
fine:

x: int = 0
print(x)
x: str = 'a'
print(x)

Redefinition is only allowed to happen in the same block and the nesting level
as the original definition. This lets us keep the implementation relatively simple,
and there will hopefully be less of a readability impact, since it's less likely that
users accidentally redefine a variable or use redefinition in confusing ways. We
may want to lift the restriction in the future, however. Function arguments can
be redefined in the body but only in non-nested blocks.

The implementation does a simple static analysis to rename variables to make
them distinct when we detect redefinition. This happens before semantic
analysis pass 1.

The new behavior is not fully backward compatible with the old behavior. For
example, here we lose type context in the second assignment:

x: List[int] = []
f(x)
x = []  # Need type annotation since this creates an independent variable

Internally we use a sequence of single quotes as a variable suffix
to represent renamed variants. The implementation provides an unmangle()
function to get the original name from a potentially mangled name.

It's still impossible to redefine final names, classes and certain other things.
The main use case is simple variables, either in single or multiple assignments.

I did some benchmarking (not with the latest version though) and the performance
impact of the new pass was minor (under 2% IIRC).

Things to do in additional PRs:

Add documentation.
Make sure we unmangle names everywhere.
Consider allowing redefinition to happen sometimes in another block.

Previously the `x` reference in the rvalue had type `Any`.

…-var

ilevkivskyi

Thanks for this huge amount of work! This will make mypy more "pythonic" and closer to the spirit of PEP 526 (annotation is not a declaration, but just a hint). Overall PR looks very good, I have a bunch of small comments (mostly requests for documentation).

I would say it is important to add docs before this will be released. Supporting "conditional" redefinition is less important (because it may require some non-trivial work). But also note that it will solve one of the current downsides:

x: object
x
x = 42
if bool():
    x = "hi"  # error here

Also an idea about

x: List[str] = []
x
x  = []  # error here

Maybe when we detect UninhabitedType, instead of immediately giving an error we can try to re-infer type of r.h.s. using type of the "previous" x (with one prime less) as a context?

ilevkivskyi · 2019-01-17T17:38:14Z

mypy/renaming.py

+    def __init__(self) -> None:
+        # Counter for labeling new blocks
+        self.block_id = 0
+        self.disallow_redef_depth = 0


Maybe add a comment documenting this attribute?

ilevkivskyi · 2019-01-17T17:40:39Z

mypy/renaming.py

+        # Counter for labeling new blocks
+        self.block_id = 0
+        self.disallow_redef_depth = 0
+        self.loop_depth = 0


This one is pretty clear, but some details would be helpful (is it for "continuous" nesting of loops or total, ie. including while (...): if(...): while (...): ... etc).

ilevkivskyi · 2019-01-17T17:41:56Z

mypy/renaming.py

+        self.block_loop_depth = {}  # type: Dict[int, int]
+        # Stack of block ids being processed.
+        self.blocks = []  # type: List[int]
+        # List of scopes; each scope maps short name to block id.


Suggested change

# List of scopes; each scope maps short name to block id.

# List of scopes; each scope maps short (i.e. unsuffixed) name to block id.

ilevkivskyi · 2019-01-17T17:42:18Z

mypy/renaming.py

+        # List of scopes; each scope maps short name to block id.
+        self.var_blocks = []  # type: List[Dict[str, int]]
+
+        # References to variables the we may need to rename. List of


Suggested change

# References to variables the we may need to rename. List of

# References to variables that we may need to rename. List of

ilevkivskyi · 2019-01-17T17:49:03Z

mypy/renaming.py

+
+        for arg in fdef.arguments:
+            name = arg.variable.name()
+            can_be_redefined = name != 'self'  # TODO: Proper check


What about 'cls'?

We special case self since it can define new attributes. cls doesn't have this property, so it doesn't need to be special cased. Added comment.

ilevkivskyi · 2019-01-17T18:20:31Z

mypy/semanal_shared.py

@@ -1,7 +1,7 @@
 """Shared definitions used by different parts of semantic analysis."""

 from abc import abstractmethod, abstractproperty
-from typing import Optional, List, Callable
+from typing import Optional, List, Callable, Dict, Set


Suggested change

from typing import Optional, List, Callable, Dict, Set

from typing import Optional, List, Callable

ilevkivskyi · 2019-01-17T18:22:56Z

mypy/semanal.py

@@ -1750,11 +1752,12 @@ def add_type_alias_deps(self, aliases_used: Iterable[str],
        self.cur_mod_node.alias_deps[target].update(aliases_used)

    def visit_assignment_stmt(self, s: AssignmentStmt) -> None:
-        self.unwrap_final(s)
+        is_final = self.unwrap_final(s)
+        s.is_final_def = is_final


is_final only used once. I would just write s.is_final_def = self.unwrap_final(s).

ilevkivskyi · 2019-01-17T18:29:38Z

mypy/semanal.py

@@ -2127,24 +2135,52 @@ def analyze_name_lvalue(self,
                # Since the is_new_def flag is set, this must have been analyzed
                # already in the first pass and added to the symbol table.
                # An exception is typing module with incomplete test fixtures.
+                if (is_final
+                        and unmangle(lval.name) + "'" in self.globals
+                        and unmangle(lval.name) + "'" != lval.name):


This looks a bit mysterious. A comment explaining why name with two primes or more are "bad".

Refactored and added a comment.

ilevkivskyi · 2019-01-17T18:40:21Z

test-data/unit/check-final.test

 x = 2  # E: Cannot assign to final name "x"
 def f() -> int:
    global x
-    x = 3  # E: Cannot assign to final name "x"
+    x = 3  # No error here is okay since we reported an error above


If one removes the assignment above, will this become an error?

Yes. Added a test case.

ilevkivskyi · 2019-01-17T19:02:24Z

test-data/unit/check-redefine.test

+    _ = 0
+    _ = ''
+    x, _ = 0, C()
+[builtins fixtures/tuple.pyi]


I would add test cases for

e = 1 e try: ... except Exception as e: ...

and

with open('file') as f: # IO[str] ... with open('file', 'b') as f: # IO[bytes] ...

(maybe plus some variations).

Added test cases.

JukkaL · 2019-01-18T17:10:56Z

I think that I've now addressed all the feedback.

Agreed that this will need to be documented by the next release (or we should hide the option).

Maybe when we detect UninhabitedType, instead of immediately giving an error we can try to re-infer type of r.h.s. using type of the "previous" x (with one prime less) as a context?

I thought about this but I couldn't come up with a simple implementation, so I decided to postpone this. Calculating the previous 'x' is more complicated, since the rules for renaming are different for globals and locals, for example. This is a potential future improvement. I'll create a follow-up issue about this.

ilevkivskyi

Thanks for the update! LGTM.

Please don't forget to create follow-up issues for the things we discussed: allowing more (conditional) redefinitions, problem with empty containers and using "previous" type, adding docs. (You also mentioned "Make sure we unmangle names everywhere" but I am not sure what is missing in this PR).

neighthan · 2020-03-25T20:27:25Z

Is there a way to only allow redefinition if an explicit type is given? So that

x = 0
print(x)
x: str = 'a'
print(x)

is valid but

x = 0
print(x)
x = 'a'
print(x)

is not? This would prevent any accidental redefinition and make it clear that the type is supposed to be changing (in the example above it's clear anyway, but if we had x = some_func() it might not be obvious anymore).

oakkitten · 2020-04-25T22:30:27Z

this doesn't seem to work for def:

from typing import Callable, cast

foo: str = "a"
foo: int = cast(int, foo)

bar: Callable[[], int] = lambda: 1
bar: int = cast(int, bar)

def baz() -> int:
    return 1
baz: int = cast(int, baz)

11: error: Name 'baz' already defined on line 9

any workarounds?

JukkaL added 30 commits September 28, 2018 12:54

Support redefinition of local variables with a different type

8e49664

Add docstring

3b409fc

Allow redefinitions of form "x = f(x)"

71cb8ea

Previously the `x` reference in the rvalue had type `Any`.

Fix special cases

35f56ae

Add test cases

2524777

Fix redefinition with 'break' and 'continue'

ab70d0d

Fix issues

525d5f4

Fix some edge cases

2c614a2

Support top-level redefinition through renaming (WIP)

bf2f869

Remove obsolete code

9e2c6b8

Cleanup

2b885da

Merge transform code into a single class

712669c

Code cleanup

e0b7df0

Fix issues

f153809

Fix issues

856d401

Work around type check error

865f1dd

Merge branch 'master' into redefine-var

2deff14

Fix merge issue

27a1e57

Fix lint

f04e37a

Fix more tests

1f9f62e

Rename semanal_redef -> renaming

9ba82a9

Only perform redefinition if variable has been read

fbf052e

A few fixes

1116b47

Add hacky workaround for 'self'

70a3085

Merge branch 'master' into redefine-var

cfad560

Remove unused things

b57960b

Revert some unnecessary changes

580a774

Merge commit '3bea26f89a66834f42caaa90cf8db44f99d39dcc' into redefine…

051e695

…-var

Merge branch 'master' into redefine-var

77495fb

Fix test case

dc03143

JukkaL added 2 commits January 15, 2019 16:17

Merge branch 'master' into redefine-var

e165e46

Merge branch 'master' into redefine-var

0a119b3

JukkaL requested a review from ilevkivskyi January 15, 2019 17:42

JukkaL added 2 commits January 17, 2019 14:22

Merge branch 'master' into redefine-var

fadb322

Fix brokenness from merge

76ad071

ilevkivskyi reviewed Jan 17, 2019

View reviewed changes

JukkaL added 10 commits January 18, 2019 11:37

Respond to review

979a17c

Clarify code

ae0043f

Test additional special case

512c1ed

Add test case

1f7e36d

Fix with statements

72043ae

Remove unnecessary check

8c30c90

Add comment

e390f38

Ensure renaming cannot happen across scopes in class

bc0ee19

Remove unnecessary call

54b86d8

The final assignments takes precedence in class when renaming

496d2dd

ilevkivskyi approved these changes Jan 18, 2019

View reviewed changes

JukkaL merged commit d2f1d34 into master Jan 21, 2019

gvanrossum deleted the redefine-var branch January 21, 2019 16:09

WillAyd mentioned this pull request Sep 16, 2019

Add Indent Support in to_json pandas-dev/pandas#28130

Merged

5 tasks

mpharrigan mentioned this pull request Feb 18, 2021

Consider --allow-redefinition for mypy quantumlib/Cirq#3832

Open

saaketp mentioned this pull request Apr 15, 2021

Support changing a variable's type microsoft/pylance-release#257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally allow redefinition of variable with different type #6197

Optionally allow redefinition of variable with different type #6197

JukkaL commented Jan 15, 2019

ilevkivskyi left a comment

ilevkivskyi Jan 17, 2019

ilevkivskyi Jan 17, 2019

ilevkivskyi Jan 17, 2019

ilevkivskyi Jan 17, 2019

ilevkivskyi Jan 17, 2019

JukkaL Jan 18, 2019

ilevkivskyi Jan 17, 2019

ilevkivskyi Jan 17, 2019

ilevkivskyi Jan 17, 2019

JukkaL Jan 18, 2019

ilevkivskyi Jan 17, 2019

JukkaL Jan 18, 2019

ilevkivskyi Jan 17, 2019

JukkaL Jan 18, 2019

JukkaL commented Jan 18, 2019

ilevkivskyi left a comment

neighthan commented Mar 25, 2020

oakkitten commented Apr 25, 2020

	# List of scopes; each scope maps short name to block id.
	# List of scopes; each scope maps short (i.e. unsuffixed) name to block id.

	# References to variables the we may need to rename. List of
	# References to variables that we may need to rename. List of

	from typing import Optional, List, Callable, Dict, Set
	from typing import Optional, List, Callable

Optionally allow redefinition of variable with different type #6197

Optionally allow redefinition of variable with different type #6197

Conversation

JukkaL commented Jan 15, 2019

ilevkivskyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JukkaL commented Jan 18, 2019

ilevkivskyi left a comment

Choose a reason for hiding this comment

neighthan commented Mar 25, 2020

oakkitten commented Apr 25, 2020